Could we rename this to just WindowHandle?

Possibly. I just don't want it to cause confusion with the RawWindowHandle crate.

This is OK for now, but we should probably change this so that ChildWindowHandle just implements HasRawWindowHandle.

Well, that works in the open_as_if_parented case, but that won't work in the open_parented case where we don't always get the RawWindowHandle. I suppose we could add a method to ChildWindowHandle that returns an Option<HasRawWindowHandle>.

Well, I think technically there's no reason we can't also provide the RawWindowHandle in the open_parented case. But I think it's fine to leave the API like this for now.

Can we just call this close(), rather than request_close()? "Request close" makes more sense to me as referring to what happens when the user clicks the X on a window, which just sends an event to the application requesting that the window be closed, whereas when the WindowHandler calls this method on the Window, the window should just be closed.

Hmm, I think wrl wanted to eventually have the ability to have the app accept or decline a close request (like pop-up a dialog that says "Do you want to save before you exit?" kind of thing).

Of course there are also occasions where the window must close, like when a DAW requests the window to close.

Yes, that ability to accept or decline would be in response to the X button being pressed, which is delivered as a particular kind of event (WM_CLOSE on Windows, windowShouldClose: on macOS, and WM_DELETE_WINDOW on X11). So that would make sense as a new WindowEvent called ShouldClose or RequestClose. That's a different situation from this method, which is the application itself telling the window to close.

This can just be a const at the top of the file; I don't think we need to keep it as a member of WindowState.

This is not a constant because we are registering a unique ID at runtime with.

let destroy_msg_id =
                RegisterWindowMessageA("Baseview::DestroyMsg\0".as_ptr() as LPCSTR);

We could do what winit does and use the lazy_static crate, but I'm not sure how well that will work in a plugin context.

Oh, I assumed you were just using a WM_USER event, which is what druid-shell does. It doesn't require any registration so might be simpler, but up to you.

Oh okay. Where does the WM_USER constant come from?

It's defined in winapi: https://docs.rs/winapi/0.3.9/winapi/um/winuser/constant.WM_USER.html

Also, could we wrap the platform ChildWindowHandle up in a struct similarly to Window, so that it's easier to catch discrepancies between the APIs on different platforms? And also with a PhantomData<*mut ()> to ensure it is !Send.

The handle is meant to be Send. It's whole purpose is when it is dropped by the parent thread it signals the window thread to close.

It will need to exist on a different thread from the window on X11 specifically, but that doesn't mean the handle itself needs to be Send. On all three platforms, there is a single thread on which all plugin API windowing operations happen, including requesting for the plugin to open and close its editor window.

So from the standpoint of Baseview's API, the ChildWindowHandle is created via Window::open_parented() on the windowing thread, it stays on the windowing thread for the lifetime of the editor, and then it gets dropped (or close() is called) on the windowing thread. The fact that specifically on X11 Baseview runs the event loop on a different thread doesn't mean that there is any need for a ChildWindowHandle to be sent between threads at any point.

Another reason to keep ChildWindowHandle !Send is that we want closing the window to be performed synchronously. On X11 we can do that using a synchronous channel or by joining the event loop thread, but on Windows and macOS that has to be done using a windowing API call, and that call should not be performed on a different thread from the window.

Ah, okay.

This shouldn't need to be an Arc<AtomicBool> on Windows, just an Rc<Cell<bool>>, since everything on Windows happens in a single thread.

Although can the DAW be on a different thread?

On Windows and Mac, in every DAW, opening and closing the editor will happen on the same thread as the window.

Whether the window is closed via Window::close() or by dropping the ChildWindowHandle, the handler doesn't need to be able to decide whether the window stays open or not, since they're already the one who decided to close it in the first place. The handler should get a choice in the case where the X button is clicked (WM_CLOSE event on Windows), which is a different thing, and explicitly has the semantics of requesting the window to be closed but allowing the application to decide.

I guess this is a matter of preference, but I personally would like it better if there were an explicit close() method on the ChildWindowHandle, and dropping the handle didn't do anything. This sort of mirrors the semantics of JoinHandle in std, where there's an explicit join() method, and dropping the handle just detaches the thread.

Hmm, I suppose so.

Could we get rid of the ParentHandle struct and just store the Rc<Cell<bool>> window_closed flag directly in WindowState (and then set it to true in WM_NCDESTROY), and then also just have Window::open directly return a ChildWindowHandle (which open_blocking can just ignore)? That will let us get rid of the awkward dance where the HWND is an option that starts out empty and then later gets set by Window::open.

Well on the other platforms the ParentHandle contains two atomic bools. I figured I would just keep it consistent.

I probably didn't explain this clearly; could you make this a wrapper struct that contains the platform::WindowHandle, similarly to Window, with the PhantomData in the outer struct? That way the public API is defined separately from the individual backends, and if a backend doesn't match it it won't build.

Ah, okay. That makes sense.

This will need to be changed from just setting an atomic to somehow synchronizing with the window being closed (using a synchronous channel, or by joining on the event loop thread). But I'm OK with merging it this way for now.

Yeah. I figured I would just do the simplest solution for now so the issue with closing and reopening windows could be fixed in Linux. I'll add a TODO comment explaining the proper solution.

Can this be renamed to is_open (with the opposite true/false values)? I think that's a lot more clear/obvious.

ok